-
Notifications
You must be signed in to change notification settings - Fork 12.1k
Add new clz(bytes)
and clz(uint256)
functions
#5725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 56da3b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
equal
, nibbles
and clz
functions to Bytes.solnibbles
and clz
functions to Bytes.sol
contracts/utils/Bytes.sol
Outdated
function nibbles(bytes memory value) internal pure returns (bytes memory) { | ||
uint256 length = value.length; | ||
bytes memory nibbles_ = new bytes(length * 2); | ||
for (uint256 i = 0; i < length; i++) { | ||
(nibbles_[i * 2], nibbles_[i * 2 + 1]) = (value[i] & 0xf0, value[i] & 0x0f); | ||
} | ||
return nibbles_; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty inefficient, consider using unchecked
at minimum.
Also unclear why this is useful:
➜ nibbles(hex"ABCD")
Type: dynamic bytes
├ Hex (Memory):
├─ Length ([0x00:0x20]): 0x0000000000000000000000000000000000000000000000000000000000000004
├─ Contents ([0x20:..]): 0xa00bc00d00000000000000000000000000000000000000000000000000000000
├ Hex (Tuple Encoded):
├─ Pointer ([0x00:0x20]): 0x0000000000000000000000000000000000000000000000000000000000000020
├─ Length ([0x20:0x40]): 0x0000000000000000000000000000000000000000000000000000000000000004
└─ Contents ([0x40:..]): 0xa00bc00d00000000000000000000000000000000000000000000000000000000
➜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unchecked
would be safe since overflow is impossible given bytes memory
can realistically only have a length smaller than 256 bits.
➜ bytes memory b = new bytes(type(uint32).max);
Traces:
[942682544] 0xBd770416a3345F91E4B34576cb804a576fa48EB1::run()
└─ ← [MemoryOOG] EvmError: MemoryOOG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd fix tests first and then optimize, it's likely that we may leverage other functions of the Bytes library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial motivation for this function is to use it in #5680 for TrieProof, but I agree that reallocating memory is perhaps not the most efficient thing to do. I suspect TrieProof may not require the nibbles()
function if these are read in place, but I need to experiment a bit with that.
We can add the unchecked, but, is there an alternative you were thinking of? I am more worried about the memory expansion cost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if inspecting the nibbles JIT rather than converting to separate nibbles array is worthwhile to explore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In regard to memory expansion, we need calldata variants for all methods related to MPT proofs (both RLP decoding and MPT branch verification) since the input data is almost always provided by the user via calldata. There's rarely a reason to copy the full RLP payload into memory, because typical use cases (like verifying a historical blockhash) only require extracting one or two fields (e.g. stateRoot, txRoot). Operating directly on calldata avoids unnecessary memory allocation and is significantly more gas-efficient. Comparing my personal implementation to RLPReader
I'm saving about 40k gas when parsing every block header field (which should never really be needed but serves as a good gas comparison and unit test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the nibbles method it may make sense to first check the size of the input, if less than or equal to 32 bytes the above calculation could be much simpler (no loop needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback @0xClandestine!
We discussed it internally and we agreed to remove the nibbles
function for now since it doesn't seem to be useful outside of the context of MPT. I'll reimplement the function privately in MPT
nibbles
and clz
functions to Bytes.solnibbles(bytes)
, clz(bytes)
and clz(uint256)
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLZ opcode EIP https://eips.ethereum.org/EIPS/eip-7939 propose to count the leading zero bits. This proposal counts leading zero bytes.
I think this could lead to a lot of confusion.
I would advice we count zero bits (like the EIP). If we get that number of zero bits, we can very easily figure out the number of zero bytes by just dividing it by 8. The other way around it not possible.
Addressed in c749346
nibbles(bytes)
, clz(bytes)
and clz(uint256)
functionsclz(bytes)
and clz(uint256)
functions
Co-authored-by: Hadrien Croubois <[email protected]>
Requires #5726
PR Checklist
npx changeset add
)